-
-
Notifications
You must be signed in to change notification settings - Fork 601
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: rename --define-process-env-node-env
to --config-node-env
#4318
Conversation
759901b
to
5544aaf
Compare
--node-env
and `--define-process-env-n…--node-env
and --define-process-env-node-env
--node-env
and --define-process-env-node-env
--node-env
and --define-process-env-node-env
} else if (typeof options.nodeEnv === "string") { | ||
const isNodeEnvOptionDefined = typeof options.nodeEnv === "string"; | ||
const isDefineProcessEnvNodeEnvOptionDefined = | ||
typeof options.defineProcessEnvNodeEnv === "string"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snitin315 I don't think we need this option anymore anyway. We have - https://github.com/webpack/webpack/blob/main/lib/WebpackOptionsApply.js#L599.
The main problem is that we have two options:
--node-env
(setprocess.env.NODE_ENV
forwebpack.config.js
)--optimization-node-env
(setprocess.env.NODE_ENV
for runtime)
And that's why they confuse some developers. The names are almost the same, but they do completely different things.
I think the best solution would be to do the following:
- remove
--define-process-env-node-env
at all - we leave
--node-env
because it is widely used by many - we come up with a new name for the
--node-env
variable and updates the documentation so that new developers can use it
Shortly --node-env
and ---new-name-node-env
work and do the same thing, but the documentation will recommend to use a new name.
To be honest I have no ideas for a new name, so I'd be happy to hear any suggestions 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexander-akait how about --config-node-env
or --config-only-node-env
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--config-node-env
sounds good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated 👍🏻
d34d6e3
to
1ee224c
Compare
--node-env
and --define-process-env-node-env
--define-process-env-node-env
to --config-node-env
What kind of change does this PR introduce?
Fix #3503 (comment)
webpack-cli/packages/webpack-cli/src/webpack-cli.ts
Lines 2388 to 2390 in c97779b
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
Will update in docs website, I have updated the option descriptions.
Summary
#4318 (comment)
Does this PR introduce a breaking change?
Yes
--define-process-env-node-env
was removed in favor of--config-node-env
option.Other information
No